Conversation
|
| }, | ||
| "dependencies": { | ||
| "graceful-fs": "^4.2.4", | ||
| "lru-cache": "^10.2.2", |
There was a problem hiding this comment.
I don't think we need this big package here, you can rewrite this package and put it inside project, because we use only the max value
| "use strict"; | ||
|
|
||
| const path = require("path"); | ||
| const { LRUCache } = require("lru-cache"); |
There was a problem hiding this comment.
Let's use your implementation here and remove this package from here
There was a problem hiding this comment.
Also there is simple and fast example:
class LRU {
constructor(max = 2048) {
this.max = max;
this.cache = new Map();
}
get(key) {
let item = this.cache.get(key);
if (item !== undefined) {
// refresh key
this.cache.delete(key);
this.cache.set(key, item);
}
return item;
}
set(key, val) {
// refresh key
if (this.cache.has(key)) this.cache.delete(key);
// evict oldest
else if (this.cache.size === this.max) this.cache.delete(this.first());
this.cache.set(key, val);
}
first() {
return this.cache.keys().next().value;
}
}
|
@vankop What do you think about it? Yeah, we have a memory leak here, if you run develpment your application very long our cache will be bigger and bigger, so I think using lru cache is good idea, but I don't know what is will be better number for max |
|
I personally think we dont need cache here.. maybe at first implementation there was some specific usage for |
|
on cold builds it does not make sense either since very low cache hit |
|
@alexander-akait sorry I'm a bit busy, I'll address it at the weekend, @vankop so do you recommend to just remove it instead? |
|
@vankop Can you test it on some repo and check it? |
|
we have a metrics repo, we just need to setup cache hits there... |
4 participants
Closes #414